Enables reload of dataloaders on every n epochs from every epoch #5043

sidhantls · 2020-12-09T17:06:28Z

What does this PR do?

This PR extends the logic of reloading dataloaders on every epoch allowing DLs to be reloaded every n epochs. It changes the argument from reload_dataloaders_every_epoch to reload_dataloaders_every_n_epochs . The parameter accepts an int>= 0. The default value is 0

Fixes #5001

Before submitting

Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
Did you read the contributor guideline, Pull Request section?
Did you make sure your PR does only one thing, instead of bundling different changes together? Otherwise, we ask you to create a separate PR for every change.
Did you make sure to update the documentation with your changes?
Did you write any new necessary tests?
Did you verify new and existing tests pass locally with your changes?
If you made a notable change (that affects users), did you update the CHANGELOG?

PR review

Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

Is this pull request ready for review? (if not, please submit in draft mode)
Check that all items from Before submitting are resolved
Make sure the title is self-explanatory and the description concisely explains the PR
Add labels and milestones (and optionally projects) to the PR so it can be classified; Bugfixes should be including in bug-fix release milestones (m.f.X) and features should be included in (m.X.b) releases.

Did you have fun?

Make sure you had fun coding 🙃

rohitgr7 · 2020-12-09T17:35:42Z

you need to deprecate reload_dataloaders_every_epoch, assign its value to reload_dataloaders_every_n_epoch and raise a deprecation warning if it's set.

Also maybe rename reload_dataloaders_every_n_epoch to reload_dataloaders_every_n_epochs??

And ofcourse pls add a test too.

tchaton

LGTM ! Minor changes and a test.

pytorch_lightning/trainer/evaluation_loop.py

pytorch_lightning/trainer/connectors/data_connector.py

pytorch_lightning/trainer/training_loop.py

tests/trainer/test_dataloaders.py

codecov · 2020-12-09T19:05:58Z

Codecov Report

Merging #5043 (e173cee) into master (bca5adf) will decrease coverage by 5%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #5043    +/-   ##
=======================================
- Coverage      92%     88%    -5%     
=======================================
  Files         213     213            
  Lines       13798   13809    +11     
=======================================
- Hits        12744   12132   -612     
- Misses       1054    1677   +623

sidhantls · 2020-12-09T20:49:15Z

@rohitgr7 after looking at check_val_every_n_epoch I just went with reload_dataloaders_every_n_epoch. but sure, I can rename it to reload_dataloaders_every_n_epochs

rohitgr7 · 2020-12-09T20:59:08Z

@rohitgr7 after looking at check_val_every_n_epoch I just went with reload_dataloaders_every_n_epoch. but sure, I can rename it to reload_dataloaders_every_n_epochs

ah.. I was just looking at these two arguments so though epochs might be better. https://github.com/PyTorchLightning/pytorch-lightning/blob/05f25f3a543dd06382feb7f19761e452af572288/pytorch_lightning/trainer/trainer.py#L111-L112

pep8speaks · 2020-12-11T15:40:37Z

Hello @sid-sundrani! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-07-07 01:18:59 UTC

pytorch_lightning/trainer/connectors/data_connector.py

tests/trainer/test_dataloaders.py

sidhantls · 2020-12-13T07:02:27Z

you need to deprecate reload_dataloaders_every_epoch, assign its value to reload_dataloaders_every_n_epoch and raise a deprecation warning if it's set.

@rohitgr7 the warning doesn't print for some reason if I use DeprecationWarning with rank_zero_warn

from pytorch_lightning.utilities import rank_zero_warn
# works, prints 
rank_zero_warn("Deprecation message..")
# doesnt print. there are a few examples of this usage 
rank_zero_warn("Deprecation message..", DeprecationWarning)

should I do the former?

rohitgr7 · 2020-12-13T07:51:51Z

you need to deprecate reload_dataloaders_every_epoch, assign its value to reload_dataloaders_every_n_epoch and raise a deprecation warning if it's set.

@rohitgr7 the warning doesn't print for some reason if I use DeprecationWarning with rank_zero_warn
from pytorch_lightning.utilities import rank_zero_warn
# works, prints 
rank_zero_warn("Deprecation message..")
# doesnt print. there are a few examples of this usage 
rank_zero_warn("Deprecation message..", DeprecationWarning)
should I do the former?

Can you add the warning at the correct place where it should be and a failing test? Will check it. @sid-sundrani

rohitgr7

Let's make it an int only, no bool. Default to 0. since other similar parameters check_val_every_n_epoch, log_every_n_steps, ... doesn't accept a bool value.

pytorch_lightning/trainer/connectors/data_connector.py

pytorch_lightning/trainer/trainer.py

pytorch_lightning/trainer/training_loop.py

tests/trainer/test_dataloaders.py

rohitgr7

you also need to update the docs here: https://github.com/PyTorchLightning/pytorch-lightning/blob/master/pytorch_lightning/core/hooks.py

Also this test too: https://github.com/PyTorchLightning/pytorch-lightning/blob/eb9cb3c8017b1f207ac1d2c7168327fbe0b63cb5/tests/core/test_datamodules.py#L437

these docs here too: https://github.com/PyTorchLightning/pytorch-lightning/blob/master/docs/source/trainer.rst

pytorch_lightning/trainer/connectors/data_connector.py

pytorch_lightning/trainer/properties.py

CHANGELOG.md

pytorch_lightning/trainer/properties.py

tests/deprecated_api/test_remove_1-4.py

tests/trainer/test_dataloaders.py

rohitgr7

LGTM. I'm just wondering if someone sets limit_train/val/test_batches=0 should it even load the dataloaders at all??

docs/source/trainer.rst

pytorch_lightning/core/hooks.py

pytorch_lightning/trainer/connectors/data_connector.py

docs/source/trainer.rst

tests/core/test_datamodules.py

rohitgr7 · 2020-12-19T13:06:52Z

@sid-sundrani pls mark it as ready for review if it's ready.

CHANGELOG.md

pytorch_lightning/trainer/trainer.py

pytorch_lightning/trainer/connectors/data_connector.py

edenlightning · 2021-07-06T20:34:17Z

@sid-sundrani any chance you can rebase and address final comments? we would love to finally merge!

pytorch_lightning/trainer/connectors/data_connector.py

tests/deprecated_api/test_remove_1-6.py

pytorch_lightning/trainer/trainer.py

pytorch_lightning/trainer/connectors/data_connector.py

tchaton reviewed Dec 9, 2020

View reviewed changes

carmocca reviewed Dec 13, 2020

View reviewed changes

rohitgr7 reviewed Dec 13, 2020

View reviewed changes

carmocca reviewed Dec 13, 2020

View reviewed changes

tests/trainer/test_dataloaders.py Outdated Show resolved Hide resolved

carmocca reviewed Dec 13, 2020

View reviewed changes

tests/trainer/test_dataloaders.py Outdated Show resolved Hide resolved

tchaton added this to the 1.2 milestone Dec 14, 2020

tchaton added design Includes a design discussion data handling Generic data-related topic labels Dec 14, 2020

sidhantls requested a review from rohitgr7 December 14, 2020 14:34

sidhantls commented Dec 14, 2020

View reviewed changes

tests/trainer/test_dataloaders.py Outdated Show resolved Hide resolved

rohitgr7 reviewed Dec 14, 2020

View reviewed changes

pytorch_lightning/trainer/connectors/data_connector.py Outdated Show resolved Hide resolved

pytorch_lightning/trainer/properties.py Outdated Show resolved Hide resolved

Borda changed the base branch from master to release/1.2-dev December 14, 2020 17:32

carmocca reviewed Dec 14, 2020

View reviewed changes

sidhantls requested a review from carmocca December 16, 2020 16:27

rohitgr7 approved these changes Dec 18, 2020

View reviewed changes

rohitgr7 reviewed Dec 18, 2020

View reviewed changes

tests/core/test_datamodules.py Show resolved Hide resolved

sidhantls marked this pull request as ready for review December 19, 2020 15:13

sidhantls requested review from awaelchli, Borda, edenlightning, justusschock and SeanNaren as code owners December 19, 2020 15:13

akihironitta requested review from carmocca and removed request for teddykoker June 24, 2021 02:54

mergify bot added the has conflicts label Jun 24, 2021

carmocca added the ready PRs ready to be merged label Jun 24, 2021

Merge branch 'master' into sid-sundrani/master

12903c4

mergify bot removed the has conflicts label Jun 24, 2021

carmocca approved these changes Jun 24, 2021

View reviewed changes

awaelchli reviewed Jun 27, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

pytorch_lightning/trainer/trainer.py Outdated Show resolved Hide resolved

pytorch_lightning/trainer/connectors/data_connector.py Outdated Show resolved Hide resolved

justusschock approved these changes Jun 28, 2021

View reviewed changes

mergify bot added the has conflicts label Jul 1, 2021

Update CHANGELOG.md

11cee0e

awaelchli reviewed Jul 6, 2021

View reviewed changes

awaelchli added 5 commits July 7, 2021 00:54

use rank_zero_deprecation

8fc2fa5

update deprecation message in trainer api docs

2cacfbb

test deprecation with real arg name in message

0a4f87f

fix typo in trainer docs

fe1ca87

Merge branch 'master' into master

e173cee

mergify bot removed the has conflicts label Jul 6, 2021

carmocca requested a review from awaelchli July 7, 2021 01:11

awaelchli approved these changes Jul 7, 2021

View reviewed changes

awaelchli enabled auto-merge (squash) July 7, 2021 01:17

awaelchli closed this Jul 7, 2021

auto-merge was automatically disabled July 7, 2021 01:18
Pull request was closed

awaelchli reopened this Jul 7, 2021

awaelchli enabled auto-merge (squash) July 7, 2021 01:19

kaushikb11 disabled auto-merge July 7, 2021 10:46

kaushikb11 changed the title ~~Enables reload of dataloaders on every n epochs from every epoch~~ Enables reload of dataloaders on every n epochs from every epoch Jul 7, 2021

kaushikb11 enabled auto-merge (squash) July 7, 2021 10:48

carmocca merged commit 20df24d into Lightning-AI:master Jul 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enables reload of dataloaders on every n epochs from every epoch #5043

Enables reload of dataloaders on every n epochs from every epoch #5043

sidhantls commented Dec 9, 2020 •

edited

Loading

rohitgr7 commented Dec 9, 2020 •

edited

Loading

tchaton left a comment

codecov bot commented Dec 9, 2020 •

edited

Loading

sidhantls commented Dec 9, 2020

rohitgr7 commented Dec 9, 2020

pep8speaks commented Dec 11, 2020 •

edited

Loading

sidhantls commented Dec 13, 2020

rohitgr7 commented Dec 13, 2020

rohitgr7 left a comment

rohitgr7 left a comment •

edited

Loading

rohitgr7 left a comment

rohitgr7 commented Dec 19, 2020

edenlightning commented Jul 6, 2021

Enables reload of dataloaders on every n epochs from every epoch #5043

Enables reload of dataloaders on every n epochs from every epoch #5043

Conversation

sidhantls commented Dec 9, 2020 • edited Loading

What does this PR do?

Before submitting

PR review

Did you have fun?

rohitgr7 commented Dec 9, 2020 • edited Loading

tchaton left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 9, 2020 • edited Loading

Codecov Report

sidhantls commented Dec 9, 2020

rohitgr7 commented Dec 9, 2020

pep8speaks commented Dec 11, 2020 • edited Loading

Comment last updated at 2021-07-07 01:18:59 UTC

sidhantls commented Dec 13, 2020

rohitgr7 commented Dec 13, 2020

rohitgr7 left a comment

Choose a reason for hiding this comment

rohitgr7 left a comment • edited Loading

Choose a reason for hiding this comment

rohitgr7 left a comment

Choose a reason for hiding this comment

rohitgr7 commented Dec 19, 2020

edenlightning commented Jul 6, 2021

sidhantls commented Dec 9, 2020 •

edited

Loading

rohitgr7 commented Dec 9, 2020 •

edited

Loading

codecov bot commented Dec 9, 2020 •

edited

Loading

pep8speaks commented Dec 11, 2020 •

edited

Loading

rohitgr7 left a comment •

edited

Loading